Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IdP Initiate SLO Validate SessionIndex #141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oss-aimoto
Copy link

Internal Server Error when IdP-Initiated Single Logout(#140)

Stores the SessionIndex in the cache, Validate SessionIndex on single logout.

Stores the SessionIndex in the cache, Validate SessionIndex on single logout.
@thijskh thijskh requested a review from simo5 April 8, 2024 06:36
@thijskh
Copy link

thijskh commented Apr 8, 2024

@simo5 Can you take a look?

cache->key[0] = '\0';
cache->access = apr_time_now();
}
am_cache_mutex_unlock(r);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is bad practice ... hiding a lock this deep (two nested) into a utility function is a sure way to get bugs later as it will be very difficult to remember and figure out.

As a general rule locking and unlocking should happen in the same calling function, so that it is very explicit what is the context of the locked code, and all error conditions can be properly checked.

Please rework this code to get the unlock out of this call and up a few levels.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. However there is a reason why I decided on this code.
The code before modification was to lock with am_get_request_session_nameid() and then unlock with am_delete_request_session().
I tried to make it consistent with the existing code.

I couldn't come up with a good fix, so I need someone else to make a better fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants